feat: Some fixes post discussions with Audit & others by dev-jodee · Pull Request #225 · solana-foundation/kora

dev-jodee · 2025-09-25T14:34:55Z

Price source should not optional anywhere
Double fee counting with ATA calculation and then fee payer outflow calculation, removed ATA separated calc completely
Checked math operations everywhere to prevent overflow errors
.unwrap() removed with better graceful error handling -remove signature from response in signAndSendTransaction and signTransaction

Important

Improves error handling, removes unnecessary ATA fee calculations, and mandates price_source in transaction processing.

Behavior:
- price_source is now mandatory in fee.rs and price.rs.
- Removed ATA fee calculation in fee.rs.
- Improved error handling by replacing .unwrap() with error checks in main.rs and auth.rs.
- Removed signature from SignAndSendTransactionResponse and SignTransactionResponse in sign_and_send_transaction.rs and sign_transaction.rs.
Fee Calculation:
- Removed ATA creation fee calculation in fee.rs.
- Added overflow checks in fee calculations in fee.rs and price.rs.
Error Handling:
- Added ConfigError to KoraError in error.rs.
- Improved error handling in main.rs and auth.rs by replacing .unwrap() with error checks.
Testing:
- Updated tests to reflect changes in transaction signing and fee calculations in transaction_signing.rs and token_2022_test.rs.
- Removed tests related to ATA fee calculation in fee.rs.
Misc:
- Updated GitHub Actions workflows to include source hash in artifact names in build-rust.yml and typescript-integration.yml.

^{This description was created by}^{for b76a1e2. You can customize this summary. It will automatically update as commits are pushed.}

📊 Unit Test Coverage

Unit Test Coverage: 84.7%

View Detailed Coverage Report

- Price source should not optional anywhere - Double fee counting with ATA calculation and then fee payer outflow calculation, removed ATA separated calc completely - Checked math operations everywhere to prevent overflow errors - .unwrap() removed with better graceful error handling -remove signature from response in signAndSendTransaction and signTransaction

github-actions · 2025-09-25T14:35:25Z

📊 TypeScript Coverage Report

Coverage: 82.0%

View detailed report

Coverage artifacts have been uploaded to this workflow run.
View Artifacts

ellipsis-dev

Important

Looks good to me! 👍

Reviewed everything up to 32d5034 in 2 minutes and 37 seconds. Click for details.

Reviewed 1520 lines of code in 20 files
Skipped 0 files when reviewing.
Skipped posting 3 draft comments. View those below.
Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. crates/lib/src/transaction/versioned_transaction.rs:167

Draft comment:
Consider handling potential errors from bs58::decode instead of using unwrap_or_default. Propagating the error would help avoid silently defaulting to empty data when instruction decoding fails.
Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.

2. sdks/ts/test/integration.test.ts:96

Draft comment:
Ensure blockhash length checks account for possible variations across environments. The test checks if blockhash length is between 43 and 44 chars, but blockhash formats may vary.
Reason this comment was not posted:
Confidence changes required: 50% <= threshold 50% None

3. crates/lib/src/fee/fee.rs:206

Draft comment:
Typo: "inclded" should be corrected to "included".
Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% The comment is about a typo in a comment, not about code logic. The rules state we should not make purely informative comments or comments that are obvious/unimportant. A typo in a comment does not affect functionality and fixing it would be a trivial change that doesn't improve code quality. Could a typo in documentation make the code harder to understand for future developers? Could this be considered a code quality issue? While clear documentation is important, a simple typo like "inclded" vs "included" is still easily understood and doesn't meaningfully impact code comprehension. This is too minor to warrant a PR comment. Delete the comment. While it points out a real typo, fixing typos in comments is too minor an issue to warrant a PR comment.

Workflow ID: wflow_5vd28JLCK6b3sooG

^{You can customize}^{by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

crates/cli/src/main.rs

crates/lib/src/fee/price.rs

sdks/ts/test/integration.test.ts

crates/lib/src/token/spl_token_2022.rs

crates/lib/src/fee/fee.rs

ellipsis-dev

Important

Looks good to me! 👍

Reviewed 1f8ba0c in 2 minutes and 20 seconds. Click for details.

Reviewed 75 lines of code in 3 files
Skipped 0 files when reviewing.
Skipped posting 5 draft comments. View those below.
Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. .github/workflows/build-rust.yml:10

Draft comment:
Updated description clarifies that the provided artifact-name is a base name; the final artifact name includes a hash suffix. Ensure that workflow consumers are aware of this behavior.
Reason this comment was not posted:
Comment did not seem useful. Confidence is useful = 0% <= threshold 50% This comment is purely informative and asks the PR author to ensure that workflow consumers are aware of a behavior. It does not provide a specific code suggestion or ask for a specific test to be written. It violates the rules against making purely informative comments and asking the author to ensure behavior is known.

2. .github/workflows/build-rust.yml:25

Draft comment:
The job-level output 'artifact-name' concatenates the input with the computed hash. Verify that the 'source-hash' step reliably produces a valid hash in all cases.
Reason this comment was not posted:
Comment did not seem useful. Confidence is useful = 0% <= threshold 50% The comment is asking the PR author to verify the reliability of a step, which is against the rules. It doesn't provide a specific suggestion or point out a clear issue with the code.

3. .github/workflows/build-rust.yml:35

Draft comment:
The 'Calculate source files hash' step uses a long find command. Consider adding inline comments or additional quoting to handle filenames with spaces, and ensure robust error handling.
Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% 1. The command is in a GitHub Actions workflow running on Ubuntu. 2. The files being searched are all source controlled files in a Rust project. 3. It's extremely unlikely for Rust source files, Cargo files, or Makefiles to have spaces in their names - this would be very unconventional. 4. The command works as is and error handling isn't critical since a failure would be immediately visible in the workflow. The comment raises valid points about shell script best practices in general. File names with spaces could theoretically exist. While technically correct, the suggestions are overly cautious given this specific context of a Rust project where space-containing filenames are extremely unlikely and would be caught immediately if they existed. The comment should be deleted as it suggests defensive programming that isn't necessary in this specific context and doesn't highlight a real problem that needs fixing.

4. .github/workflows/rust.yml:61

Draft comment:
Replaced the static artifact name with the dynamic output from the build job. This improves consistency, but ensure that the build job always outputs a valid artifact name.
Reason this comment was not posted:
Comment did not seem useful. Confidence is useful = 0% <= threshold 50% The comment is informative and suggests ensuring that the build job outputs a valid artifact name. It doesn't provide a specific code suggestion or ask for a specific test to be written. It violates the rule against asking the PR author to ensure behavior is intended or tested.

5. .github/workflows/typescript-integration.yml:69

Draft comment:
Updated the artifact download step to use the dynamic artifact name. Confirm that the build job’s output corresponds correctly when using 'rust-binaries-ts' as the base input.
Reason this comment was not posted:
Comment did not seem useful. Confidence is useful = 0% <= threshold 50% The comment is asking the PR author to confirm that the build job's output corresponds correctly with a specific input. This falls under the rule of not asking the author to confirm their intention or ensure behavior is intended. Therefore, this comment should be removed.

Workflow ID: wflow_V1XZVBdCtF6MyV20

^{You can customize}^{by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

ellipsis-dev

Important

Looks good to me! 👍

Reviewed 6d68b83 in 1 minute and 0 seconds. Click for details.

Reviewed 34 lines of code in 1 files
Skipped 0 files when reviewing.
Skipped posting 1 draft comments. View those below.
Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. crates/lib/src/fee/price.rs:168

Draft comment:
Removed obsolete 'test_fixed_model_get_required_lamports_without_oracle'. Since PriceSource is now required everywhere, this test case is no longer needed.
Reason this comment was not posted:
Confidence changes required: 0% <= threshold 50% None

Workflow ID: wflow_qZs86OXIPHkasu7j

^{You can customize}^{by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

ellipsis-dev

Caution

Changes requested ❌

Reviewed b76a1e2 in 3 minutes and 50 seconds. Click for details.

Reviewed 591 lines of code in 3 files
Skipped 0 files when reviewing.
Skipped posting 12 draft comments. View those below.
Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. crates/lib/src/rpc_server/openapi/spec/combined_api.json:64

Draft comment:
Consider revising the description for the 'signer_key' property to remove the redundant wording ('Optional signer signer_key') for clarity.
Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% While the comment identifies a real issue with redundant wording, it's a very minor documentation improvement. The current wording, while not ideal, is still understandable and doesn't impact functionality. The rules state we should only keep comments that require clear code changes, and documentation tweaks like this are not critical enough to warrant a comment. The wording is genuinely redundant and could be clearer. Improving API documentation can be valuable for developers using the API. However, the improvement is extremely minor and doesn't affect functionality. The documentation is still understandable despite the redundancy. Delete the comment as it suggests a very minor documentation improvement that doesn't meet the threshold for requiring a comment.

2. sdks/ts/src/types/index.ts:31

Draft comment:
The comment for 'sig_verify' in TS types indicates a default of false, but the OpenAPI spec says default true. Ensure the default behavior is consistent across client and API docs.
Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.

3. sdks/ts/src/types/index.ts:231

Draft comment:
There is a naming mismatch: the ValidationConfig interface uses 'token2022' while the OpenAPI spec uses 'token_2022'. This discrepancy may cause parsing issues.
Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.

4. sdks/ts/test/unit.test.ts:261

Draft comment:
The unit test for signTransactionIfPaid conditionally adds a 'margin' property, yet the corresponding TS interface and OpenAPI schema do not define 'margin'. This inconsistency should be resolved.
Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.

5. crates/lib/src/rpc_server/openapi/spec/combined_api.json:69

Draft comment:
Typographical issue: The description contains 'Optional signer signer_key', which appears to be a duplicated 'signer'. Consider updating the description to 'Optional signer key to ensure consistency across related RPC calls' or similar.
Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% While the comment is technically correct about the duplication, this is a very minor documentation issue. The meaning is still clear despite the typo. The rules state not to make purely informative comments or comments that are obvious/unimportant. Documentation typos that don't impact functionality would fall into this category. The typo could potentially confuse API users since it appears in the public API documentation. Documentation quality does matter for APIs. While documentation quality matters, this particular typo is extremely minor and doesn't impact understanding - readers would easily understand the meaning despite the duplicated word. The rules specifically say not to make unimportant comments. The comment should be deleted as it addresses a very minor documentation issue that doesn't impact functionality or understanding.

6. crates/lib/src/rpc_server/openapi/spec/combined_api.json:590

Draft comment:
Typographical error: The description for the 'signer_key' property contains a redundant word. Consider changing "Optional signer signer_key to ensure consistency across related RPC calls" to "Optional signer key to ensure consistency across related RPC calls".
Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% While the comment is technically correct about the redundant word, this is a very minor issue in an API spec description field. The meaning is still clear despite the redundancy. The rules state not to make comments that are obvious or unimportant, and this seems like a very minor wording issue that doesn't impact functionality. The redundant word could potentially cause confusion for API users reading the documentation. Clear documentation is important for APIs. While clear documentation is important, this minor wording redundancy doesn't meaningfully impact understanding of the API. The meaning remains clear despite the redundant word. This comment should be deleted as it addresses a very minor documentation wording issue that doesn't impact functionality or clarity in a meaningful way.

7. crates/lib/src/rpc_server/openapi/spec/combined_api.json:829

Draft comment:
Typo: The description repeats the word 'signer' ('Optional signer signer_key ...'). Consider removing the redundant 'signer' for clarity.
Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% While the comment is technically correct about the word repetition, this is a very minor documentation issue. The meaning is still clear even with the repeated word. The rules state we should not make purely informative comments or comments that are obvious/unimportant. Documentation typos that don't affect functionality fall into this category. The repeated word could potentially cause confusion for API users reading the documentation. Documentation clarity is important for APIs. While clear documentation is important, this particular typo is extremely minor and doesn't meaningfully impact understanding. The meaning is still obvious to readers. Delete the comment. The typo is too minor to warrant a PR comment, and we should avoid nitpicking documentation when the meaning remains clear.

8. crates/lib/src/rpc_server/openapi/spec/combined_api.json:960

Draft comment:
There's a duplicated word in the description. Consider revising "Optional signer signer_key to ensure consistency across related RPC calls" to remove the redundancy (e.g., "Optional signer key to ensure consistency across related RPC calls").
Reason this comment was not posted:
Comment did not seem useful. Confidence is useful = 20% <= threshold 50% The comment is pointing out a redundancy in the description, which is not directly related to the code functionality or logic. It is more of a stylistic suggestion rather than a necessary code review comment. According to the rules, comments should not ask the PR author to update the PR description or focus on purely informative aspects.

9. crates/lib/src/rpc_server/openapi/spec/combined_api.json:1284

Draft comment:
Typo: The description for 'signer_key' contains a repeated word 'signer' ('Optional signer signer_key'). Please remove the duplicate word to fix the typo.
Reason this comment was not posted:
Marked as duplicate.

10. crates/lib/src/rpc_server/openapi/spec/combined_api.json:1320

Draft comment:
Typo: The description for the 'signer_key' property contains a duplicate word ('Optional signer signer_key'). Consider removing the extra 'signer' to read something like 'Optional signer key to ensure consistency across related RPC calls'.
Reason this comment was not posted:
Marked as duplicate.

11. crates/lib/src/rpc_server/openapi/spec/combined_api.json:1360

Draft comment:
Typographical suggestion: In the description for signer_key, the phrase "Optional signer signer_key" appears redundant. Consider revising it (e.g., "Optional signer key to ensure consistency across related RPC calls") to remove the duplicate "signer".
Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% While the comment is technically correct about the redundant wording, the PR review rules state that we should not make purely informative comments or comments that are obvious/unimportant. This is a very minor wording issue in API documentation that doesn't affect functionality. The meaning is still clear despite the redundant word. The redundant word could theoretically cause confusion for API users reading the documentation. Documentation clarity is important for APIs. While documentation clarity matters, this particular redundancy is extremely minor and doesn't meaningfully impact understanding. The rules explicitly say not to make unimportant comments. This comment should be deleted as it addresses a trivial wording issue that doesn't materially impact understanding of the API.

12. crates/lib/src/rpc_server/openapi/spec/combined_api.json:1424

Draft comment:
Typographical suggestion: The description text "Optional signer signer_key to ensure consistency across related RPC calls" appears to have a redundant use of 'signer'. Consider rephrasing it to something like "Optional signer key to ensure consistency across related RPC calls".
Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% While the comment is technically correct about the redundant word, the review rules state not to make purely informative comments or comments that are obvious/unimportant. This is a very minor wording issue in API documentation that doesn't affect functionality. The meaning is still clear despite the redundant word. The redundant word could potentially cause minor confusion for API users reading the docs. The suggestion does make the text clearer. However, this is an extremely minor documentation wording issue that doesn't impact functionality or significantly impair understanding. The review rules explicitly say not to make purely informative comments. Delete this comment as it's a minor documentation wording issue that doesn't warrant a PR comment per the review rules about avoiding purely informative comments.

Workflow ID: wflow_cvw3fVbt8gZHIwEI

^{You can customize}^{by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

crates/lib/src/rpc_server/openapi/spec/combined_api.json

dev-jodee requested a review from amilz September 25, 2025 14:34

ellipsis-dev bot reviewed Sep 25, 2025

View reviewed changes

amilz reviewed Sep 25, 2025

View reviewed changes

crates/lib/src/fee/fee.rs Show resolved Hide resolved

dev-jodee added 2 commits September 25, 2025 12:40

Pr fixes

e1858b2

CI fixes

1f8ba0c

ellipsis-dev bot reviewed Sep 25, 2025

View reviewed changes

Fix testing with rpc not optional

6d68b83

ellipsis-dev bot reviewed Sep 25, 2025

View reviewed changes

Updated ts types and docs

b76a1e2

ellipsis-dev bot reviewed Sep 25, 2025

View reviewed changes

crates/lib/src/rpc_server/openapi/spec/combined_api.json Show resolved Hide resolved

dev-jodee merged commit 01f69c2 into release/feature-freeze-for-audit Sep 25, 2025
13 of 15 checks passed

dev-jodee deleted the feat/some-fixes-post-discussions branch January 6, 2026 19:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Some fixes post discussions with Audit & others#225

feat: Some fixes post discussions with Audit & others#225
dev-jodee merged 5 commits intorelease/feature-freeze-for-auditfrom
feat/some-fixes-post-discussions

dev-jodee commented Sep 25, 2025 •

edited by github-actions bot

Loading

Uh oh!

github-actions bot commented Sep 25, 2025 •

edited

Loading

Uh oh!

ellipsis-dev bot left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ellipsis-dev bot left a comment

Uh oh!

ellipsis-dev bot left a comment

Uh oh!

ellipsis-dev bot left a comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

dev-jodee commented Sep 25, 2025 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📊 Unit Test Coverage

Uh oh!

github-actions bot commented Sep 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📊 TypeScript Coverage Report

Uh oh!

ellipsis-dev bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ellipsis-dev bot left a comment

Choose a reason for hiding this comment

Uh oh!

ellipsis-dev bot left a comment

Choose a reason for hiding this comment

Uh oh!

ellipsis-dev bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

dev-jodee commented Sep 25, 2025 •

edited by github-actions bot

Loading

github-actions bot commented Sep 25, 2025 •

edited

Loading